Skip to content

Fix memory leak in openssl_sign() when passing invalid algorithm#18185

Closed
ndossche wants to merge 1 commit into
php:PHP-8.3from
ndossche:openssl-leak-inv-alg
Closed

Fix memory leak in openssl_sign() when passing invalid algorithm#18185
ndossche wants to merge 1 commit into
php:PHP-8.3from
ndossche:openssl-leak-inv-alg

Conversation

@ndossche

Copy link
Copy Markdown
Member

Detected using an experimental static analysis I'm developing.

@devnexen devnexen left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ndossche ndossche closed this in 74720a2 Apr 2, 2025
@jrfnl

jrfnl commented Aug 4, 2025

Copy link
Copy Markdown
Contributor

I have a feeling this commit is causing breakage on PHP 8.5 as the openssl_sign() $algorithm parameter appears to no longer support aliases.

This code:

openssl_sign($signHeader, $signature, $privKey, 'sha256WithRSAEncryption');

Now fails with the following error:

openssl_sign(): Unknown digest algorithm

Found via: https://github.com/PHPMailer/PHPMailer/actions/runs/16728360948/job/47350003979#step:9:33

@ndossche

ndossche commented Aug 4, 2025

Copy link
Copy Markdown
Member Author

@jrfnl I highly doubt it. I can run a bisect in half an hour or so though

@jrfnl

jrfnl commented Aug 4, 2025

Copy link
Copy Markdown
Contributor

@nielsdos That would be appreciated!

I agree that based on the code in this PR it seems unlikely, but it was the only commit I could find for PHP 8.5 which related to the openssl_sign() method and the last passing build in PHPMailer is from a couple of days before this was merged and the first failing one after this was merged, so the timing seems right, which is why I left the comment.

@jrfnl

jrfnl commented Aug 4, 2025

Copy link
Copy Markdown
Contributor

@nielsdos Looking more closely - you're right, it can't be this commit - the (failing) CI builds from a PR were confusing the issue.

Last passing is actually on May 31st, first failing on June 18th. Still mystifying though why the alias no longer works. I couldn't find anything in UPGRADING or NEWS related to this.

Shall I open a bug report instead ?

@ndossche

ndossche commented Aug 4, 2025

Copy link
Copy Markdown
Member Author

@jrfnl It can also be an environment change, e.g. an update to the OpenSSL library itself. You may open a bug report.

@ndossche

ndossche commented Aug 4, 2025

Copy link
Copy Markdown
Member Author

Nope, works in 8.4 breaks in 8.5, I'll bisect...

@jrfnl

jrfnl commented Aug 4, 2025

Copy link
Copy Markdown
Contributor

@nielsdos I've gone through all commits in PHP master between May 31st and June 18th and my current suspicion is that this is the culprit: #18516

I previously already verified via var_export(openssl_get_md_methods(true)); and the alias still exists and the output is not really different between PHP 8.4 and 8.5.

@ndossche

ndossche commented Aug 4, 2025

Copy link
Copy Markdown
Member Author

@jrfnl You're correct, bisect finished and points to 2f5ef4d

@jrfnl

jrfnl commented Aug 4, 2025

Copy link
Copy Markdown
Contributor

@nielsdos Thank you for doing that and confirming! In the mean time, I've set up a phpt test for alias support in openssl_sign() - I'm waiting to verify the test works as intended and if so, I'll include it in the bug report.

@jrfnl

jrfnl commented Aug 4, 2025

Copy link
Copy Markdown
Contributor

@nielsdos I've opened the ticket now: #19369

@ratatatKE

Copy link
Copy Markdown

Great job @nielsdos for catching this memory leak. Are you planning to opensource the experimental static analysis tool that you used to catch this one?

@ndossche

ndossche commented Aug 4, 2025

Copy link
Copy Markdown
Member Author

Great job @nielsdos for catching this memory leak. Are you planning to opensource the experimental static analysis tool that you used to catch this one?

That's the plan eventually, the paper describing the analysis tool is currently under review

@ratatatKE

Copy link
Copy Markdown

Great job @nielsdos for catching this memory leak. Are you planning to opensource the experimental static analysis tool that you used to catch this one?

That's the plan eventually, the paper describing the analysis tool is currently under review

Okay, where can I get my hands/eyes on the pre-release? Is it something online I can find?

@ndossche

ndossche commented Aug 5, 2025

Copy link
Copy Markdown
Member Author

Okay, where can I get my hands/eyes on the pre-release? Is it something online I can find?

It's not public yet, it will only become public once the journal publishes the article and source code (after it got reviewer approval).

@ratatatKE

Copy link
Copy Markdown

Okay @nielsdos, thanks for the update. Looking forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants